Cleanup client interface #662
Conversation
Signed-off-by: Tim Ramlot <[email protected]>
| return nil | ||
| } | ||
|
|
||
| if config.OrganizationID == "" { |
There was a problem hiding this comment.
This will not be true when TLSPKMode is JetstackSecureOAuth or JetstackSecureAPIToken, since this is checked here:
jetstack-secure/pkg/agent/config.go
Lines 598 to 600 in 5aab733
There was a problem hiding this comment.
And the other two modes (VenafiCloudKeypair and VenafiCloudVenafiConnection are handled above already).
There was a problem hiding this comment.
I imagine that the test coverage would show that this code path is never run? Either way, this doesn't seem to be tested anyways. I'm fine with removing this.
inteon
force-pushed
the
cleanup_part2
branch
2 times, most recently
from
5539c6e to
6312622
Compare
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
Cleans up the client interface by removing unused methods, consolidating HTTP helpers, and updating the agent’s data-posting logic.
- Flattened repeated failure checks in the mock identity server.
- Dropped
PostDataReadings/Postfrom the client interface and renamed helpers to unexported methods (postDataReadings,post). - Refactored
postDatain the agent to use a switch onTLSPKModeand always callPostDataReadingsWithOptions.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/internal/cyberark/identity/mock.go | Combined multiple if blocks into a single OR-based check in handleAdvanceAuthentication. |
| pkg/client/client_venconn.go | Removed unused PostDataReadings and Post methods from VenConnClient. |
| pkg/client/client_venafi_cloud.go | Swapped exported Post calls to unexported post and merged reading uploads into one call. |
| pkg/client/client_oauth.go | Renamed PostDataReadings and Post to unexported helpers for consistency. |
| pkg/client/client_api_token.go | Same renaming of public helpers to unexported methods. |
| pkg/client/client.go | Deleted PostDataReadings and Post from the Client interface. |
| pkg/agent/run.go | Rewrote postData to switch on TLSPKMode and route through PostDataReadingsWithOptions. |
Comments suppressed due to low confidence (3)
pkg/client/client_venafi_cloud.go:226
- Update this comment to reference
post(the new method name) instead ofPostto keep docs in sync with the code.
// Post performs an HTTP POST request.
pkg/client/client_api_token.go:86
- The method was renamed to
post, so this doc comment should be updated to avoid confusion.
// Post performs an HTTP POST request.
pkg/agent/run.go:453
- Add a unit or integration test for the default
TLSPKModebranch to verify that it returns the expected "not implemented" error.
default:
wallrj
left a comment
wallrj
left a comment
There was a problem hiding this comment.
Yes, I find the new code easier to understand.
/approve
/lgtm
PostDataReadingsfunction from the client interface, sincePostDataReadingsWithOptionscan be used instead.Postfunction from the client interface, since it was not used (config.OrganizationIDcannot be"")